MDEV-37990: redundant Table map events in binlog#5121
Conversation
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses MDEV-37990 by preventing redundant Table_map events in the binary log during Galera replication. It introduces a mechanism in the THD class to track mapped tables within a transaction. The review feedback identifies several areas for improvement: correcting MariaDB coding style and return types in sql/log.cc, fixing a regex bug in the test script that prevents correct transaction boundary detection, and suggesting the use of a more lightweight container than std::unordered_set in the core sql_class.h header to optimize performance and compilation times.
| #ifdef WITH_WSREP | ||
| if (wsrep_thd_is_applying(this) | ||
| && !wsrep_mark_table_mapped(table->s->table_map_id)) { | ||
| /* A table map event for this table is already recorded for this | ||
| transaction, skip writing the table map event. */ | ||
| DBUG_RETURN(0); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
The indentation and brace placement in this block do not follow the MariaDB coding style. The block inside the if statement should be indented by 2 spaces relative to the if, and the opening brace { is typically placed on a new line. Additionally, for a function returning bool, false is more idiomatic than 0 to indicate success.
#ifdef WITH_WSREP
if (wsrep_thd_is_applying(this) &&
!wsrep_mark_table_mapped(table->s->table_map_id))
{
/* A table map event for this table is already recorded for this
transaction, skip writing the table map event. */
DBUG_RETURN(false);
}
#endifReferences
- MariaDB coding style: 2-space indentation, opening braces on a new line for blocks, and use 'false' for boolean success in functions returning bool. (link)
| my (%per_txn, $xid, %out); | ||
| $xid = 0; | ||
| while (my $line = <$fh>) { | ||
| $xid++ if $line =~ /^(?:COMMIT|Xid =)/; |
There was a problem hiding this comment.
The regex /^(?:COMMIT|Xid =)/ is anchored to the start of the line, but Xid = in mysqlbinlog output is typically preceded by metadata and a tab. This will prevent the regex from correctly identifying transaction boundaries for XID events. Removing the ^ anchor will allow it to match correctly.
$xid++ if $line =~ /COMMIT|Xid =/;
| #include "wsrep_client_state.h" | ||
| #include "wsrep_mutex.h" | ||
| #include "wsrep_condition_variable.h" | ||
| #include <unordered_set> |
There was a problem hiding this comment.
Including <unordered_set> in a core header like sql_class.h can increase compilation times. Additionally, std::unordered_set is relatively heavy for tracking a small number of tables. Consider using a more lightweight container like Prealloced_array or std::vector with linear search, which would be more efficient for the typical case and avoid unnecessary heap allocations.
This patch modifies how a Galera slave node writes Table map events to binary log. Now a Galera slave does not write duplicate Table Map events to binary log.
3 participants
This patch modifies how a Galera slave node writes Table map events to binary log. Now a Galera slave does not write duplicate Table Map events to binary log.